core: Move replay + error correction logic to commit log #554
Conversation
kim
force-pushed
the
kim/move-replay-to-commit-log
branch
from
498bda7 to
f779cac
Compare
kim
force-pushed
the
kim/move-replay-to-commit-log
branch
from
f779cac to
9ce0c02
Compare
kim
force-pushed
the
kim/move-replay-to-commit-log
branch
from
9ce0c02 to
7d57673
Compare
| /// Similar to [`Iter`], but performs integrity checking and maintains | ||
| /// additional state. | ||
| #[must_use = "iterators are lazy and do nothing unless consumed"] | ||
| struct Replay { |
There was a problem hiding this comment.
It would be nice to replace Iter with this, so the error detection is available by default. However, Iter is supposed to be able to start iteration at a specified commit offset, so this is not currently possible.
| // We are near the end of the log, so trim it to the known- | ||
| // good prefix. | ||
| Err( | ||
| ReplayError::OutOfOrder { last_commit_offset, .. } |
There was a problem hiding this comment.
I understand if the last commit is corrupted, it makes sense to drop it, but it's unclear the failure scenario that would lead to an out of order commit. And so I would be hesitant to just ignore it.
There was a problem hiding this comment.
Now I understand the failure scenarios, however I think I'm even more of the opinion that we should not ignore an OutOfOrder error here, as it potentially points to a legitimate bug within the database.
| // good prefix. | ||
| Err( | ||
| ReplayError::OutOfOrder { last_commit_offset, .. } | ||
| | ReplayError::CorruptedData { last_commit_offset, .. }, |
There was a problem hiding this comment.
This is the last segment, but not necessarily the last commit right? Is there any reason not to require that corruption be confined to the last commit?
There was a problem hiding this comment.
We can't know if it is the last commit exactly -- MessageLog::open_segment_max_offset (meaning: total number of entries) is calculated without decoding the payload or checking integrity, it only means that the number of bytes is aligned.
Since commits are variable-length, we could only apply a heuristic such as "not more than ~50KiB remaining".
A viable approach for a more robust solution might be to calculate a CRC32 for every record, and allow to skip over out-of-order commits iff the next commit has the right sequence number (although I don't know how this could arise).
kim
force-pushed
the
kim/move-replay-to-commit-log
branch
from
03005dc to
80b6341
Compare
Makes `CommitLog` a read-only handle and introduces `CommitLogMut`, allowing to append. This better encapsulates the error detection and trimming logic, and enforces it to be executed before being able to mutate the log.
kim
force-pushed
the
kim/move-replay-to-commit-log
branch
from
80b6341 to
c38cceb
Compare
There was a problem hiding this comment.
This looks good to me @kim, aside from my comment about ignoring OutOfOrder errors. But I'll let @kulakowski have the final approval.
| /// We may in the future verify the commit hash, and include expected and | ||
| /// actual value in this variant. | ||
| /// | ||
| /// [465]: https://github.com/clockworklabs/SpacetimeDB/pull/465 |
There was a problem hiding this comment.
I'm a bag fan of leaving these kind of break crumbs!
crates/core/src/db/commit_log.rs
Outdated
| } | ||
|
|
||
| impl CommitLogMut { | ||
| pub fn set_fsync(&mut self, fsync: bool) { |
There was a problem hiding this comment.
Could this get a doc comment describing its postcondition? Just from the name, I could imagine two useful ones:
- All future writes will happen according to
fsync, but any prior writes may be left up to the OS to actually flush (ifself.fsyncwasfalse) - All future writes will happen according to
fsync. Additionally, flush now.
This looks unused, so it's hard to guess which is more useful.
There was a problem hiding this comment.
Oh ok, I'll fix the TODO then locally (use FsyncPolicy instead of bool)
So what’s your opinion on that? I’ll point out that if we don’t trim the log for any reason, the database will be left unusable. So what we’d gain is to preserve corrupt data for forensics or hypothetical future tooling to repair it offline. For that purpose, we could also just make a copy of the corrupt segment before truncating. Or, we could start the database with the prefix but prevent any writes. |
|
It's a little hard to tell what Joshua meant because his commentary came in a couple comments and its hard for me to tell which comment of his is responding to which. I agree that right now, trimming is the correct thing. And later if we want we can make it fancier, like you say. What I meant, though, was for you, Kim, to either agree or disagree with "as it potentially points to a legitimate bug within the database." |
|
Ok, so I think the ambiguity of the out-of-order case is an artifact of the on-disk format, and should inform future amendments to it (the format). Meanwhile, we're maintaining the ordering invariant in the same file, so I added another heads-up comment. It could be made more robust by transferring ownership of |
Description of Changes
Makes
CommitLoga read-only handle and introducesCommitLogMut,allowing to append. This better encapsulates the error detection and
trimming logic, and enforces it to be executed before being able to
mutate the log.
Stacked on top of #527CompareAPI and ABI breaking changes
Expected complexity level and risk
2